Skip to content

Use released version of distributed.#54

Merged
jhamman merged 1 commit into
dask:masterfrom
lesteve:use-released-distributed
May 14, 2018
Merged

Use released version of distributed.#54
jhamman merged 1 commit into
dask:masterfrom
lesteve:use-released-distributed

Conversation

@lesteve

@lesteve lesteve commented May 7, 2018

Copy link
Copy Markdown
Member

At the time of writing you need to have both dask and distributed master because of the config overhaul (dask/distributed#1948 and dask/dask#3432). If you only have distributed from master you'll have an error like this:

❯ python -c 'from distributed import client'                    
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/lesteve/miniconda3/envs/tmp/lib/python3.6/site-packages/distributed/__init__.py", line 3, in <module>
    from . import config
  File "/home/lesteve/miniconda3/envs/tmp/lib/python3.6/site-packages/distributed/config.py", line 13, in <module>
    config = dask.config.config
AttributeError: module 'dask' has no attribute 'config'

Also this seems like a better idea to test against released version of distributed
in our CI, as the end user is very likely going to be using a released version of distributed.

Given that there was a relase of distributed 4 days ago, I reckon this is recent enough. We can always revert to testing against distributed if we need bleeding-edge features but I would rather fix master first.

@lesteve
Use released version of distributed.
668475a 
At the time of writing you need to have both dask and distributed master. Also
this seems like a better idea to test against lreleased version of distributed
in our CI.
@mrocklin

mrocklin commented May 7, 2018

Copy link
Copy Markdown
Member

See also #48 (comment)

@guillaumeeb

guillaumeeb commented May 7, 2018

Copy link
Copy Markdown
Member

So this seems to be in conflict with current @mrocklin proposal in #48.
I apparently had to use distributed master in the PR about PBS Docker CI, see comment #47 (comment).

I'm not against having dependency against master version of distributed or dask for now, as dask-jobqueue is quite new. In the long run, I would recommend as @lesteve to be dependant and test against released versions.

@lesteve

lesteve commented May 7, 2018

Copy link
Copy Markdown
Member Author

Right I missed this about #48. To be honest, I don't have a strong opinion about this, I would just like master to be fixed.

For some context: I was trying #47 locally and bumped into this problem.

@mrocklin

mrocklin commented May 7, 2018

Copy link
Copy Markdown
Member

I think that this PR is correct and that we should go ahead and merge regardless of what happens in #48 . That discussion could take a while and it shouldn't blog progress in the meantime. Getting in CI for PBS seems higher priority to me than the config changes.

That being said, I'd love it if people would try out the configuration system and see if it is in an improvement or not. The more quickly we get feedback like this from downstream projects like dask-jobqueue and dask-kubernetes (see equivalent PR there: dask/dask-kubernetes#75) the more quickly we can solidify things into a release.

@guillaumeeb

guillaumeeb commented May 7, 2018

Copy link
Copy Markdown
Member

And @lesteve, with this fix don't you run into this kind of failure: https://travis-ci.org/dask/dask-jobqueue/jobs/372327569.
This is why I had to stick with distributed master branch in #47.

@lesteve

lesteve commented May 8, 2018

Copy link
Copy Markdown
Member Author

And @lesteve, with this fix don't you run into this kind of failure: https://travis-ci.org/dask/dask-jobqueue/jobs/372327569.

The latest distributed release is 4 days old. I am betting on the fact that the bug-fix that was in master when you were working on #47 has been included in the release.

@guillaumeeb

guillaumeeb commented May 8, 2018

Copy link
Copy Markdown
Member

The latest distributed release is 4 days old. I am betting on the fact that the bug-fix that was in master when you were working on #47 has been included in the release.

You are absolutely right.

@guillaumeeb guillaumeeb mentioned this pull request May 14, 2018
Merged
guillaumeeb
guillaumeeb approved these changes May 14, 2018
View reviewed changes

@guillaumeeb guillaumeeb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should merge this one.

@jhamman jhamman merged commit 495dc9b into dask:master May 14, 2018
guillaumeeb added a commit to guillaumeeb/dask-jobqueue that referenced this pull request May 15, 2018
@guillaumeeb
Use released versions of distributed as per dask#54
ac46240
@guillaumeeb guillaumeeb mentioned this pull request May 15, 2018
Merged
@lesteve lesteve deleted the use-released-distributed branch August 29, 2018 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guillaumeeb guillaumeeb guillaumeeb approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lesteve @mrocklin @guillaumeeb @jhamman